fix(pr-reviewer): read PR state from GitHub VFS instead of gh CLI#73
Conversation
The sandbox snapshot ships no `gh` binary, so the two `gh pr view` calls added by the READY-sentinel guard (e981031) and merge-on-green (#57) failed every run with `spawn gh ENOENT` — breaking the ready ping and auto-merge. Rebuild the same PullRequestReadyState the gates consume from the github adapter's VFS projection instead: • pulls/{n}/meta.json → state, draft, labels, head.sha • pulls/{n}/checks/_summary.json → the aggregated CI rollup • pulls/{n}/reviews/*.json → latest review per author The adapter doesn't project mergeability, so conflict detection is delegated to the merge API (mergePullRequest throws on a dirty PR — never auto-merges). All reads fail closed: a missing/empty projection yields a state the gates treat as not-ready/pending, never a green light. The tested pure evaluators are unchanged; only the data source moved off gh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesVFS-backed PR review state pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the dependency on the gh CLI by reading PR metadata, check summaries, and reviews directly from the GitHub VFS projection. Feedback on these changes suggests adding defensive guards in loadReviews to prevent runtime errors when mapping files, and making rollupFromCheckSummary more robust by calculating the total checks from individual statuses if the total field is missing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 'listReviews', | ||
| `/github/repos/${encodeSegment(pr.owner)}/${encodeSegment(pr.repo)}/pulls/${pr.number}/reviews` | ||
| ); | ||
| return files.map((file) => file.value); |
There was a problem hiding this comment.
In loadReviews, mapping files directly using files.map((file) => file.value) can throw a TypeError if files is not an array, if any file is null/undefined, or if file.value is missing. Since this function is wrapped in a try-catch block, any such error will cause it to silently fail and return []. This could lead to bypassing active CHANGES_REQUESTED reviews and incorrectly merging a PR. We should defensively guard the mapping to ensure we process valid reviews even if some elements are malformed.
return Array.isArray(files)
? files.map((file) => file?.value).filter((val): val is Record<string, unknown> => val !== null && typeof val === 'object')
: [];| const total = typeof summary?.total === 'number' ? summary.total : 0; | ||
| if (!summary || total === 0) return []; | ||
| const failed = typeof summary.failed === 'number' ? summary.failed : 0; | ||
| const pending = typeof summary.pending === 'number' ? summary.pending : 0; | ||
| const passed = typeof summary.passed === 'number' ? summary.passed : 0; |
There was a problem hiding this comment.
In rollupFromCheckSummary, if summary is defined but total is missing or not a number, total defaults to 0, which causes the function to immediately return []. If there are actually failed, pending, or passed checks present in the summary but total is omitted or malformed, they will be ignored, and the PR will be held with a generic reason instead of reporting the specific check statuses. We can make this more robust by defaulting total to the sum of failed, pending, and passed when it is not explicitly provided as a number.
| const total = typeof summary?.total === 'number' ? summary.total : 0; | |
| if (!summary || total === 0) return []; | |
| const failed = typeof summary.failed === 'number' ? summary.failed : 0; | |
| const pending = typeof summary.pending === 'number' ? summary.pending : 0; | |
| const passed = typeof summary.passed === 'number' ? summary.passed : 0; | |
| const failed = typeof summary?.failed === 'number' ? summary.failed : 0; | |
| const pending = typeof summary?.pending === 'number' ? summary.pending : 0; | |
| const passed = typeof summary?.passed === 'number' ? summary.passed : 0; | |
| const total = typeof summary?.total === 'number' ? summary.total : (failed + pending + passed); | |
| if (!summary || total === 0) return []; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6163f8119e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The VFS carries no mergeability field, so default to MERGEABLE rather than | ||
| // gate on a value we can't read. A real conflict never auto-merges: the | ||
| // merge API rejects it and mergePr throws. | ||
| mergeable: 'MERGEABLE', |
There was a problem hiding this comment.
When the VFS state is used for a PR that currently has merge conflicts, this hard-coded MERGEABLE value makes both prReadyStateAllowsHumanReview and evaluateMergeOnGreenState treat the PR as conflict-free once labels/checks/reviews are otherwise green. The merge API may still reject merge-on-green later, but the READY path does not call that API, so a conflicted PR can incorrectly ping Slack as ready for human review; merge-on-green also attempts a merge instead of holding with a conflict reason.
Useful? React with 👍 / 👎.
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. PR #73 Review —
|
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. I made no file edits — the diff is mechanically clean (typecheck + all 108 tests pass), and the only substantive findings are semantic/safety behavior changes that require human judgment, so I left the code unchanged. Review: PR #73 —
|
- loadReviews: guard the map so a malformed reviews/*.json entry can't throw out and blank the whole review set (which would drop an active CHANGES_REQUESTED and risk merging a blocked PR). - rollupFromCheckSummary: derive `total` from failed+pending+passed when it's missing/malformed, so a summary with real counts but no `total` still reports the actual check states instead of being treated as "no checks". - Document why mergeable defaults to MERGEABLE (VFS doesn't project it) and that safety rests on the merge API rejecting a conflicted PR, not on this field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review feedback in
typecheck clean; 37/37 review tests pass. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/review-agent.test.mjs (1)
237-242: ⚡ Quick winAdd malformed-
totalcoverage for fallback behavior.Line 237–242 validates missing
total, but not malformedtotal. Add one case liketotal: '3'to ensure fallback still blocks whenfailed > 0, matching the hardened parser contract.Suggested test addition
assert.equal(evaluateMergeOnGreenState({ state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', labels: [{ name: 'merge-on-green' }], statusCheckRollup: rollupFromCheckSummary({ failed: 1, pending: 0, passed: 2 }), }).outcome, 'blocked'); + + assert.equal(evaluateMergeOnGreenState({ + state: 'OPEN', isDraft: false, mergeable: 'MERGEABLE', labels: [{ name: 'merge-on-green' }], + statusCheckRollup: rollupFromCheckSummary({ total: '3', failed: 1, pending: 0, passed: 2 }), + }).outcome, 'blocked');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/review-agent.test.mjs` around lines 237 - 242, The test case for evaluateMergeOnGreenState only validates the behavior when total is missing, but not when total is malformed (like a string value). Add another similar test case using evaluateMergeOnGreenState that passes a malformed total field such as total: '3' (as a string) in the statusCheckRollup created via rollupFromCheckSummary or similar mechanism, and verify that the outcome is still 'blocked' when failed check count is greater than zero, ensuring the parser fallback behavior works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/review-agent.test.mjs`:
- Around line 237-242: The test case for evaluateMergeOnGreenState only
validates the behavior when total is missing, but not when total is malformed
(like a string value). Add another similar test case using
evaluateMergeOnGreenState that passes a malformed total field such as total: '3'
(as a string) in the statusCheckRollup created via rollupFromCheckSummary or
similar mechanism, and verify that the outcome is still 'blocked' when failed
check count is greater than zero, ensuring the parser fallback behavior works
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 742a48b9-67fe-478b-a287-0bc256917458
📒 Files selected for processing (2)
review/agent.tstests/review-agent.test.mjs
Reconcile #61 (opt-in comment-driven merge-conflict resolution) with #73 (VFS-based PR-state machinery): - review/agent.ts: union github triggers (kept issues.labeled + slack from #73, added issue_comment.created from #61); kept #73's readPrReviewState/loadCheckSummary/loadReviews/rollupFromCheckSummary/ deriveReviewDecision and #61's resolveConflicts/matchesConflictDirective/ isAuthorizedConflictCommander; merged readPr to handle both issue_comment and issues.labeled payloads (single superset issue type). - tests/review-agent.test.mjs: unioned imports so both PRs' cases coexist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Problem
Every pr-reviewer run was failing with
spawn gh ENOENT. The sandbox snapshot ships noghbinary, but two code paths shelled out togh pr view:verifyReadyForHumanReview(controls the Slack "ready" ping) — added ine981031(READY-sentinel CI guard)readMergeOnGreenState(controls auto-merge) — added in AR-99: add pr-reviewer merge-on-green handling #57 (merge-on-green)Before those commits pr-reviewer never used
gh. The harness commits via the cloud writeback and is explicitly "no git/gh" — these calls violated that.Fix
Rebuild the same
PullRequestReadyStatethe gate evaluators consume from the GitHub adapter's VFS projection instead ofgh:pulls/{n}/meta.jsonpulls/{n}/checks/_summary.json({total,passed,failed,pending})pulls/{n}/reviews/*.jsonmergePullRequest()returnsmerged:falseon a dirty PR andmergePrthrows, so a conflicting PR can never auto-merge.evaluateMergeOnGreenState,prReadyStateAllowsHumanReview) are unchanged; only the data source moved.Behavior note
The merge-on-green gate now drops the "wait for a requested bot reviewer to approve" check (the VFS doesn't project
reviewRequests). It still blocks on any bot's explicitCHANGES_REQUESTED, themerge-on-greenlabel, and all-checks-green.Test
npm run typecheckcleanrollupFromCheckSummary,deriveReviewDecision)checks/_summary.json/listReviewsand zeroghreferences🤖 Generated with Claude Code
Summary by cubic
Read PR state from the GitHub adapter VFS instead of
ghto fix sandbox failures and restore ready pings and merge-on-green. Also hardens review/check parsing so malformed VFS entries never make a PR appear green.PullRequestReadyStatefrom VFS:pulls/{n}/meta.json(state/draft/labels/head SHA),pulls/{n}/checks/_summary.json→statusCheckRollup, andpulls/{n}/reviews/*.json→ latest review per author.reviews/*.jsonentries and derivetotalfromfailed+pending+passedwhen missing so failing/pending checks still block.reviewRequestsin VFS). Still blocks on any botCHANGES_REQUESTED, requires themerge-on-greenlabel, and all checks green.Written for commit 4ab397c. Summary will update on new commits.